-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix get_jobs method in Comparison Monitor #444
Conversation
aacecaf
to
f12a87c
Compare
Force pushed branch again to remove incorrectly staged files |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #444 +/- ##
==========================================
+ Coverage 79.37% 79.39% +0.01%
==========================================
Files 76 76
Lines 3234 3237 +3
Branches 537 537
==========================================
+ Hits 2567 2570 +3
Misses 595 595
Partials 72 72 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me!
Thanks for addressing these and the CI issue 🙇♀️
start=start, | ||
state=states, | ||
count=number_of_jobs, | ||
count=count, | ||
filters=dict(has_tag=tags) if tags else None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (non-blocking): This was already like this, so we might need to update it in another PR, but filters=dict(has_tag=tags) if tags else None
doesn't do anything, at least in the version of scrapinghub
I'm using (2.4.0
). has_tag=tags
as a parameter by itself, without the filters
wrap, does filter the jobs, though I'm not sure if that's backward compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any other issues about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any other issues about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that I know of, no! And that issue is not related to these changes. We should be good to merge 👍
Fixes #442